Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add require.on function to loader. Taken example from Dojo 1.0. Related to Issue #14 #43

Closed
wants to merge 30 commits into from

Conversation

tomdye
Copy link
Member

@tomdye tomdye commented Feb 19, 2016

Fixes: #14 #42
Pre-req: #46

Can subscribe to loader errors using require.on('error', callback). This returns an object with a remove function which cancels the subscription.
When an error occurs loading a module in the loader the signal function checks to see if there are any subscribers to loader errors, if there are, it calls their callback functions, otherwise an error is thrown.

@tomdye tomdye changed the title Issue #14: added require.on function, utilising example form dojo 1.0 loader Add require.on function to loader. Taken example from Dojo 1.0. Related to Issue #14 Feb 23, 2016
@kitsonk kitsonk self-assigned this Feb 24, 2016
@kitsonk kitsonk assigned bryanforbes and unassigned kitsonk Feb 25, 2016
@@ -28,6 +28,11 @@ export interface Has {
add(name: string, value: any, now?: boolean, force?: boolean): void;
}

export interface LoaderError extends Error {
src: string;
info: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have an interface rather than using any.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the interface is simply an object, does that suffice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an object, yes, but you're passing specific things with that object. From the call later in the file:

                            makeSignalError(message, {
                                module,
                                url,
                                parentMid
                            });

Will the info be different for different errors? Will there always be some properties that will be there?

@@ -1,12 +1,18 @@
import * as assert from 'intern/chai!assert';
import * as registerSuite from 'intern!object';

interface LoaderError extends Error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be imported from src/loader.ts. Since it's just an interface, the compiler is smart enough to know that the import only needs to be done within the compiler and won't be added to the resulting JS.

});
};

const signal = function(type: SignalType, args: {}): number | boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this emit? It would match what most JS event libraries are calling this functionality these days.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, however, it isn't exposed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. But for readability's sake and understanding for future generations, emit makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

// TODO are we still abbreviating these properties?
export interface Module extends LoaderPlugin {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to our Style Guide, module exports should be ordered alphabetically.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meant to do that, will do now

@@ -1,136 +1,31 @@
import {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to import from a d.ts. It should just be ambient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to import anything. If the interface.d.ts is in the scope of the compiler, these types should already be available to you.

@tomdye
Copy link
Member Author

tomdye commented Mar 2, 2016

Added dependency to #46 by merging in TS1.8 branch

@codecov-io
Copy link

Current coverage is 86.71%

Merging #43 into master will increase coverage by +1.20% as of 45b8b20

@@            master     #43   diff @@
======================================
  Files            1       1       
  Stmts          435     459    +24
  Branches       105     109     +4
  Methods         64      69     +5
======================================
+ Hit            372     398    +26
  Partial          3       3       
+ Missed          60      58     -2

Review entire Coverage Diff as of 45b8b20

Powered by Codecov. Updated on successful CI builds.

@tomdye
Copy link
Member Author

tomdye commented Mar 2, 2016

@kitsonk @bryanforbes updated PR to include changes from TS1.8 branch.

@kitsonk kitsonk closed this in f607f68 Mar 3, 2016
@tomdye tomdye deleted the moduleLoadErrorReporting branch March 3, 2016 12:43
@dylans dylans added this to the beta.3 milestone Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants